Sync up with upstream ES main: apply project ID resolver to ScriptService. #357
Conversation
…ied in logstash-bridge ScriptServiceBridge.
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
| engines.put(PainlessScriptEngine.NAME, getPainlessScriptEngine(settings)); | ||
| engines.put(MustacheScriptEngine.NAME, new MustacheScriptEngine(settings)); | ||
| return new ScriptService(settings, engines, ScriptModule.CORE_CONTEXTS, threadPool::absoluteTimeInMillis); | ||
| return new ScriptService(settings, engines, ScriptModule.CORE_CONTEXTS, threadPool::absoluteTimeInMillis, null); |
There was a problem hiding this comment.
Ah, i see in the bridge https://github.com/elastic/elasticsearch/pull/130578/files#diff-f74baacde87eae5c09e26daf35f888065d68238e2593160179fb937c11d11327R72-R73 we explicitly set this to null.
There was a problem hiding this comment.
Oh, ha i just read the PR description 😅 :
Applying as introduced in the logstash-bridge ScriptServiceBridge: https://github.com/elastic/elasticsearch/pull/130578/files#diff-f74baacde87eae5c09e26daf35f888065d68238e2593160179fb937c11d11327R73
There was a problem hiding this comment.
| 3 PDT | logstash-1 | org.elasticsearch.ElasticsearchException: java.lang.NullPointerException: Cannot invoke "org.elasticsearch.cluster.project.ProjectResolver.getProjectId()" because "this.projectResolver" is null |
|---|
Hmm maybe that wont work
There was a problem hiding this comment.
Yeah, but it seems our case is non-null tolerant (see below error). I will do couple of tests with logstash-bridge changes (with my ongoing PR) and introduce DefaultProjectResolver.INSTANCE in the bridge as well.
2025-08-05 14:29:13 PDT | logstash-1 \| Caused by: java.lang.NullPointerException: Cannot invoke "org.elasticsearch.cluster.project.ProjectResolver.getProjectId()" because "this.projectResolver" is null
-- | --
| 2025-08-05 14:29:13 PDT | logstash-1 \| at org.elasticsearch.script.ScriptService.compile(ScriptService.java:596) ~[elasticsearch-9.2.0-SNAPSHOT.jar:9.2.0-SNAPSHOT]
| 2025-08-05 14:29:13 PDT | logstash-1 \| at org.elasticsearch.ingest.ConditionalProcessor.<init>(ConditionalProcessor.java:82) ~[elasticsearch-9.2.0-SNAPSHOT.jar:9.2.0-SNAPSHOT]
| 2025-08-05 14:29:13 PDT | logstash-1 \| at org.elasticsearch.ingest.ConditionalProcessor.<init>(ConditionalProcessor.java:62) ~[elasticsearch-9.2.0-SNAPSHOT.jar:9.2.0-SNAPSHOT]
| 2025-08-05 14:29:13 PDT | logstash-1 \| at org.elasticsearch.ingest.ConfigurationUtils.readProcessor(ConfigurationUtils.java:658) ~[elasticsearch-9.2.0-SNAPSHOT.jar:9.2.0-SNAPSHOT]
| 2025-08-05 14:29:13 PDT | logstash-1 \| ... 104 more
There was a problem hiding this comment.
Both null and DefaultProjectResolver.INSTANCE didn't work for this case. DefaultProjectResolver.INSTANCE requires additional classes such as Metadata class loading where plugin needs to include its jar package. Introducing a separate plugin based project resolver seems a better approach for now.
FYI: I have also tested with the logstash-bridge - mashhurs/elasticsearch@78a5fb9
donoghuc
dismissed
their stale review
Tests failing due to null pointer exception
💚 Build Succeeded
History
|
| engines, | ||
| ScriptModule.CORE_CONTEXTS, | ||
| threadPool::absoluteTimeInMillis, | ||
| new PluginProjectResolver()); |
There was a problem hiding this comment.
Why do we have to write out own project resolver? Can we use the default? https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/cluster/project/DefaultProjectResolver.java
Seems like our own one is missing some methods and i'm not sure what the implications of returning null will be for a project id.
There was a problem hiding this comment.
- DefaultProjectResolver.java has more dependencies (as I checked obvious one was
Metadata). Since this plugin doesn't include all ES module JARs, we get not class found exceptions. And when addressing@FixForMultiProjectin ES code, this will be discussion point with the ES team how LS needs to resolve project of currently running ingest pipeline. I have placed notes with my upcoming changes: https://github.com/elastic/elasticsearch/compare/main...mashhurs:elasticsearch:logstash-bridge-geoip-interfaces?expand=1 nullalso breaks the plugins as we do see NPE can happen when scripts get compiled: https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/script/ScriptService.java#L596
Thus, current better approach looks to me applying default project ID with minimal requirements (similar to DefaultProjectResolver except default methods)
There was a problem hiding this comment.
Ah, yep. Lets see what they say on that!
There was a problem hiding this comment.
FYI: In the logstash-bridge module, there is a wrapper for Metadata and we can introduce more if we get to know how PluginProjectResolver should behave. This is for now a minimal change to bring the plugin into a working state.
donoghuc
left a comment
donoghuc
left a comment
There was a problem hiding this comment.
I think this is worth it to get the plugin working. We can update if the pattern we choose upstream changes. 
3 participants
Sync up with upstream ES main: apply project ID resolver applied in logstash-bridge ScriptServiceBridge.
Applying as introduced in the logstash-bridge ScriptServiceBridge: https://github.com/elastic/elasticsearch/pull/130578/files#diff-f74baacde87eae5c09e26daf35f888065d68238e2593160179fb937c11d11327R73
Soon, this plugin will start using logstash-bridge instead directSee the observations that applyingScriptServiceusage.nullbreaks the plugin, so we may need to apply a logic to provide an actual project resolver.